fix(strix): supply sibling SQL migrations as PR-scope context#608
fix(strix): supply sibling SQL migrations as PR-scope context#608seonghobae wants to merge 5 commits into
Conversation
Diff-scoped Strix scans copy only the PR's changed files into the scan target. A migration that only ALTERs or references a table created by an earlier, unchanged migration therefore appears — in isolation — to touch a nonexistent relation, and the model reports a phantom CRITICAL "relation/table does not exist" finding whose PoC depends entirely on files excluded from scope. Extend pull_request_scope_context_files with a generic rule (no project-specific paths): for any changed `*/migrations/*.sql`, enumerate the sibling `.sql` files in that migrations directory from the PR head via a read-only `git ls-tree` and emit them as context. Emitted paths flow through the existing trusted PR-head copy path, so no untrusted content or exec bit is introduced. Enumeration fails open — an unavailable/invalid head SHA adds no context and leaves the base changed-file scan unchanged. Reproduced against ContextualWisdomLab/gyeot#11, whose only migration change (0003) was flagged CRITICAL because 0001/0002 (which create the table) were outside the scope. Adds a static assertion and a functional test exercising a two-migration directory with only the second file changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Strix quick gate’s PR-scope context selection so SQL migrations are scanned with their sibling migrations available as context, reducing false-positive “relation/table does not exist” findings when only a later migration is changed.
Changes:
- Extend
pull_request_scope_context_files()to detect touchedmigrations/*.sqlpaths and emit sibling.sqlfiles from the PR head for additional scan context. - Add assertions and a functional self-test ensuring sibling migrations are enumerated and included when only a later migration is changed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/ci/strix_quick_gate.sh | Collect touched migrations directories and enumerate sibling .sql files via git ls-tree for PR-scope scan context. |
| scripts/ci/test_strix_quick_gate.sh | Add static assertions + a functional regression test for sibling migration context emission. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…hub_models fallback When the primary model uses openai_direct mode, LLM_API_BASE_FILE is not set. github_models/* fallback models route through STRIX_GITHUB_MODELS_API_BASE_FILE. The old github_models_api_base_is_active() only checked LLM_API_BASE_FILE, so it returned false for openai_direct primaries, causing the gate to retry a rate-limited github_models fallback 3x (wasting ~3 minutes) instead of skipping directly to the next fallback. Fix: check STRIX_GITHUB_MODELS_API_BASE_FILE as a fallback when LLM_API_BASE_FILE is unset, mirroring the pattern already used in resolved_llm_api_base_for_model(). The retry-skip guard in github_models_rate_limit_should_skip_same_model_retry() already gates on is_github_models_api_compatible_model(), so openai_direct primary models (which don't match github_models/* patterns) are unaffected. Add test scenario openai-direct-github-models-fallback-ratelimit-skip covering the bug: primary retries 3x (openai_direct, not GitHub Models), then the first github_models fallback hits rate limit and correctly skips retry (1 attempt), and the second fallback succeeds (5 total strix calls, not 7).
The same explanation is already in the standalone invocation below. Removing the redundant multi-line comment from the filter dispatch case.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
scripts/ci/strix_quick_gate.sh:1215
- The loop over
sql_migration_dirsuses an unnecessary${arr[@]+"${arr[@]}"}expansion without quoting theforlist, which makes the code harder to reason about and can mis-handle directory names containing whitespace or glob characters. Sincesql_migration_dirsis always initialized (local -a sql_migration_dirs=()), you can iterate safely with a normal quoted array expansion.
local seen_migration_dir=0 known_migration_dir
for known_migration_dir in ${sql_migration_dirs[@]+"${sql_migration_dirs[@]}"}; do
if [ "$known_migration_dir" = "$migration_dir" ]; then
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/ci/strix_quick_gate.sh:1214
- The dedupe loop iterates over sql_migration_dirs using an unquoted parameter-expansion pattern. If a migrations directory path contains spaces (allowed by normalize_changed_file_path), word-splitting can break the equality check and lead to duplicate dirs or missed matches. Iterate over the array directly with proper quoting.
local seen_migration_dir=0 known_migration_dir
for known_migration_dir in ${sql_migration_dirs[@]+"${sql_migration_dirs[@]}"}; do
scripts/ci/strix_quick_gate.sh:1318
- git ls-tree output is affected by core.quotepath; for non-ASCII paths it may emit quoted/escaped names containing backslashes. Those paths will be rejected by normalize_changed_file_path later, silently dropping sibling migrations from context. Use
-c core.quotepath=falsehere (as done elsewhere in this script) so paths are emitted verbatim.
git ls-tree -r --name-only "$head_sha_for_migration_context" -- "$migration_context_dir/" 2>/dev/null |
scripts/ci/test_strix_quick_gate.sh:170
- This assertion hard-codes the git ls-tree invocation string. If the gate switches to
git -c core.quotepath=false ls-tree ...(to avoid quoted/escaped paths), update this expected substring accordingly so the self-test continues to validate the behavior.
assert_file_contains "$GATE_SCRIPT" "git ls-tree -r --name-only \"\$head_sha_for_migration_context\" -- \"\$migration_context_dir/\"" "strix gate enumerates sibling migrations from the PR head"
문제
Strix 필수 게이트는 PR의 변경 파일만 스캔 스코프로 복사합니다(
__PR_SCOPE__). 이전 마이그레이션이 만든 테이블을ALTER/참조만 하는 마이그레이션 한 개가 바뀌면, 격리된 스코프에서는 그 테이블이 존재하지 않는 것처럼 보여 모델이 "relation/table does not exist" 유령 CRITICAL을 보고합니다. 이 finding의 PoC는 전적으로 스코프에서 제외된 파일(형제 마이그레이션)에 의존합니다.재현
ContextualWisdomLab/gyeot#11— 마이그레이션 변경은0003_cbt_envelope.sql(ALTER TABLE momentary_response ...) 하나뿐인데, 테이블을 만드는0001_init.sql/0002_auth.sql이 스코프 밖이라 **VULN-0005 CRITICAL(CVSS 9.1) "migration 0003 references a nonexistent base table"**가 떴습니다. 실제로는 3개 마이그레이션을 모두 적용하면 정상 부팅합니다(해당 repo의test_server.sh가 Docker Postgres에서 PASS).변경
pull_request_scope_context_files에 범용 규칙(프로젝트 전용 경로 하드코딩 없음)을 추가:*/migrations/*.sql이면 그 마이그레이션 디렉토리를 수집하고,git ls-tree(읽기 전용)로 형제.sql을 열거해 컨텍스트로 방출.방출 경로는 기존 신뢰 PR-head 복사 경로를 그대로 타므로 비신뢰 콘텐츠·실행 비트가 유입되지 않습니다. 열거는 fail-open — head SHA가 없거나 유효하지 않으면 컨텍스트를 추가하지 않고 기존 changed-file 스캔에 영향이 없습니다. 게이트/심각도/신뢰 로직은 불변(추가만).
테스트
bash -n통과, 함수 격리 실행에서 형제 마이그레이션 방출 확인, 신규 어서션 3/3 PASS.🤖 Generated with Claude Code